New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Engine and Golang support for shimless providers #10916
Conversation
Changelog[uncommitted] (2022-11-14)Features
|
0c5d7f1
to
e7b4767
Compare
02735df
to
dee259a
Compare
.vscode/settings.json
Outdated
@@ -1,5 +1,5 @@ | |||
{ | |||
"go.buildTags": "all smoke", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stopped "run test" widgets working in non-smoke test files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a PR to fix this - I think I've just forgotten to merge it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Queued: #10879
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that fails to merge for any reason, I'll pull the change into a separate PR.
9893164
to
aa4cd71
Compare
func MakeInstallDependenciesStreams( | ||
server pulumirpc.LanguageRuntime_InstallDependenciesServer, | ||
isTerminal bool) (io.Closer, io.Writer, io.Writer, error) { | ||
|
||
return makeStreams( | ||
func(b []byte) error { | ||
return server.Send(&pulumirpc.InstallDependenciesResponse{Stdout: b}) | ||
}, | ||
func(b []byte) error { | ||
return server.Send(&pulumirpc.InstallDependenciesResponse{Stderr: b}) | ||
}, | ||
isTerminal) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will npm
continue to render a progress bar (re: #10901) on unix-y platforms, at least? It looks like this indirection shouldn't change the behavior.
It seems like it would be a pain to write a test to verify, so I'll just ask to confirm. (Not sure what we could test, could we snoop the stream and detect if it's sending VT commands?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh this shouldn't change the current InstallDependencies behavior, I just ended up with a slightly different proto-layout. Given we update all these things in lock-step I could probably do a sanitize pass over this to bring everything into the same system, just need to do the dance with the yaml and java repos so wouldn't want to do it just before a release day.
46c22f3
to
aa4cd71
Compare
aa4cd71
to
ff38be7
Compare
@@ -39,6 +39,9 @@ service LanguageRuntime { | |||
|
|||
// GetProgramDependencies returns the set of dependencies required by the program. | |||
rpc GetProgramDependencies(GetProgramDependenciesRequest) returns (GetProgramDependenciesResponse) {} | |||
|
|||
// RunPlugin executes a plugin program and returns its result asynchronously. | |||
rpc RunPlugin(RunPluginRequest) returns (stream RunPluginResponse) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frustrating that we can't have a tuple of immediate data and a trailing stream in a single RPC. The response API will look really funny if we want to add a message type for communicating a structured failure starting the plugin outside of the context of running it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh I really wish grpc streams were a three type message of (immediate response message, many stream messages, final terminating message), but we work with what we've got.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I'm curious about the test skip.
@@ -663,6 +663,9 @@ func testComponentProviderSchema(t *testing.T, path string) { | |||
test := test | |||
t.Run(test.name, func(t *testing.T) { | |||
t.Parallel() | |||
|
|||
t.Skip("This needs to use a plugin host to deal with non-native-binary providers") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this test being skipped when it previously passed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this actually only matters for Go so I'll move the skip to just that test (in tests/integration/integration_go_test.go) but it's because this method is attempting to exec the plugin binary directly and then manually wrapping the grpc machinery around it, but I've changed component_setup.sh so we don't actually build the go component anymore so there is no native binary for it to exec.
I'll just skip the go test for now, and I'll rewrite this test to use the plugin host machinery properly before the node and python components are similarly stripped down.
ff38be7
to
b19be5f
Compare
b19be5f
to
f7ccf38
Compare
This allows the pulumi-language-go plugin to start up providers directly from .go source files. The other language providers will be extended to support this as well in time.
f7ccf38
to
9e5f1cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm happy with these changes.
bors merge |
Build succeeded: |
Description
This allows the pulumi-language-go plugin to start up providers directly from .go source files.
The other language providers will be extended to support this as well in time.
Checklist
make changelog
and committed thechangelog/pending/<file>
documenting my change